Add Concurrency option and MPSC support using farbot. by atsushieno · Pull Request #18 · cjappl/rtlog-cpp

atsushieno · 2024-12-17T07:15:15Z

context: #6 #14

It would not be a fun task to make up a customization setup for queue backends, so I made a minimum-ish one. The behavior should be backward compatible.

context: cjappl#6 cjappl#14

cjappl

Thank you for taking the time for this work!! Let's chat more about some of my comments and see what we can come up with. I like the direction it is going, but we should work out some of the bigger picture.

cjappl · 2024-12-20T18:55:07Z

test/test_rtlog.cpp


  SUBCASE("Enqueue more than capacity and get an error") {
-    const auto maxNumMessages = 10;
+    const auto maxNumMessages = 16;


Is there a reason this changed? I'm surprised

(I'm going to bed soon-ish, so let me put quick comments only for now.)

Yes - farbot FIFO does not accept the size of buffers that is not 2^n. Probably I should put some caution on the README or the header or somewhere.

Maybe we can enforce it via a static assert/restriction on the template param

I don't think we use this class for now, but I have added static_assert in the MPSC wrapper constructor.

cjappl · 2024-12-20T18:57:53Z

CMakeLists.txt

    FetchContent_MakeAvailable(ReaderWriterQueue)
 endif()

+if (NOT TARGET farbot)


THANK YOU for this contribution and taking the time to write it up.

I have a couple of concerns, which maybe you can help me figure out.

First, I never liked how I downloaded concurrentqueue "secretly" for the users. I think a (maybe better?) approach would be the users providing their own queue, perhaps wrapped in some External Polymorphism package like you've done here.

Now that we have two options (farbot AND concurrentqueue) we are downloading even more dependencies "secretly" for the user. It feels a little dirty, and I don't know what we should do about it.

Perhaps one solution to this is to pass in the queue that the suer wants, but just provide two back ends that can be hidden behind some optional cmake

RTSAN_BUILD_FARBOT_BACKEND
RTSAN_BUILD_CONCURRENTQUEUE_BACKEND

I'm completely spitballing here. I just want to make sure before we extend to 2 backends that we have a plan for when we inevitably need to add a 3rd

I understand your concern. Once thing I would point out is that we would probably like to have those options alongside and not exclusive - one would like to have both kinds of loggers (also to avoid possible build config conflicts between sub-projects).

(BTW I didn't replace readerwriterqueue with concurrentqueue, so only farbot version is added. I can add that too if you want, but it's not going to be RT log anymore ;-) )

I'm digging into this just a little bit and I remember one of the sticking points that prevented me from doing this in the past.

If you change the args to have a QueueType:

template <typename LogData, size_t MaxNumMessages, size_t MaxMessageLength, std::atomic<std::size_t>& SequenceNumber, typename QueueType>

This becomes tricky because QueueType needs to depend on InternalLogData.

(I'm not sure immediately how to re-arrange to get around this problem)

cjappl · 2024-12-20T18:59:02Z

include/rtlog/rtlog.h


    InternalLogData value;
-    while (mQueue.try_dequeue(value)) {
+    while (mQueue->tryDequeue(value)) {


My other slight concern is this use of dynamic memory here. While it is perfectly real-time safe, it is also slower (probably) than the one that holds the queue on the stack.

Is there any way to have this be template based instead of inheritance based?

Alright. It is probably possible. We would still need those wrapper classes to make code consistent though (yet removing inheritance).

cjappl · 2024-12-20T19:04:22Z

include/rtlog/rtlog.h

+  };
+  class InternalQueueMPSC : public InternalQueue {
+    farbot::fifo<InternalLogData, farbot::fifo_options::concurrency::single,
+                 farbot::fifo_options::concurrency::multiple>


Smaller comment that we should address after the bigger comments above - let's make the behavior on push/pop explicit:

fifo_options::full_empty_failure_mode::return_false_on_full_or_empty, fifo_options::full_empty_failure_mode::overwrite_or_return_default

Are the options. I think both are wait free on write, I guess I would opt for return_false_on_full_or_empty so the user can decide what they want to do if the queue fails and data loss happens

cjappl · 2024-12-20T19:05:26Z

include/rtlog/rtlog.h

+      Concurrency == QueueConcurrency::Single_Producer_Single_Consumer
+          ? (std::unique_ptr<InternalQueue>)
+                std::make_unique<InternalQueueSPSC>()
+          : std::make_unique<InternalQueueMPSC>()};


nit:

perhaps a case statement would be more clear and more extensible for the future

cjappl · 2024-12-20T19:05:40Z

test/test_rtlog.cpp


 constexpr auto MAX_LOG_MESSAGE_LENGTH = 256;
-constexpr auto MAX_NUM_LOG_MESSAGES = 100;
+constexpr auto MAX_NUM_LOG_MESSAGES = 128;


Is there a reason this changed?

Yes, same as maxNumMessages.

cjappl · 2024-12-21T16:51:23Z

@atsushieno - would you check out #19 when you get a chance?

Possibly even pull it down and see if you can easily use it with farbot? This is just a draft of how this COULD work, not necessarily the final version

atsushieno · 2024-12-22T08:11:00Z

I'm trying plain #19 without my changes, but I couldn't get this compiled on my macOS environment:

====================[ Build | all | Debug ]=====================================
/Users/atsushi/Applications/CLion.app/Contents/bin/cmake/mac/aarch64/bin/cmake --build /Users/atsushi/sources/rtlog-cpp/cmake-build-debug --target all -j 6
[1/2] Building CXX object examples/everlog/CMakeFiles/everlog.dir/everlogmain.cpp.o
FAILED: examples/everlog/CMakeFiles/everlog.dir/everlogmain.cpp.o 
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ -DDEBUG -DRTLOG_USE_FMTLIB -DSTB_SPRINTF_IMPLEMENTATION -I/Users/atsushi/sources/rtlog-cpp/include -I/Users/atsushi/sources/rtlog-cpp/cmake-build-debug/_deps/readerwriterqueue-src -I/Users/atsushi/sources/rtlog-cpp/cmake-build-debug/_deps/stb-src -I/Users/atsushi/sources/rtlog-cpp/cmake-build-debug/_deps/fmtlib-src/include -g -std=gnu++17 -arch arm64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX15.0.sdk -mmacosx-version-min=14.7 -fcolor-diagnostics -MD -MT examples/everlog/CMakeFiles/everlog.dir/everlogmain.cpp.o -MF examples/everlog/CMakeFiles/everlog.dir/everlogmain.cpp.o.d -o examples/everlog/CMakeFiles/everlog.dir/everlogmain.cpp.o -c /Users/atsushi/sources/rtlog-cpp/examples/everlog/everlogmain.cpp
In file included from /Users/atsushi/sources/rtlog-cpp/examples/everlog/everlogmain.cpp:1:
/Users/atsushi/sources/rtlog-cpp/include/rtlog/rtlog.h:103:57: error: template template argument has different template parameters than its corresponding template template parameter
  103 |           template <typename> class QType = moodycamel::ReaderWriterQueue>
      |                                                         ^
/Users/atsushi/sources/rtlog-cpp/examples/everlog/everlogmain.cpp:109:37: note: while checking a default template argument used here
  108 | static rtlog::Logger<LogData, MAX_NUM_LOG_MESSAGES, MAX_LOG_MESSAGE_LENGTH,
      |               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  109 |                      gSequenceNumber>
      |                      ~~~~~~~~~~~~~~~^
/Users/atsushi/sources/rtlog-cpp/cmake-build-debug/_deps/readerwriterqueue-src/readerwriterqueue.h:75:1: note: too many template parameters in template template argument
   75 | template<typename T, size_t MAX_BLOCK_SIZE = 512>
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/atsushi/sources/rtlog-cpp/include/rtlog/rtlog.h:103:11: note: previous template template parameter is here
  103 |           template <typename> class QType = moodycamel::ReaderWriterQueue>
      |           ^~~~~~~~~~~~~~~~~~~
1 error generated.
ninja: build stopped: subcommand failed.

I'm not sure how portable the use of those template template parameters are. (I guess it should be fairly standard?)

atsushieno · 2024-12-22T10:37:36Z

According to my experiment at Compiler Explorer, clang earlier than 19 had this issue. clang-19 successfully compiles it. https://godbolt.org/z/o755bnvPh

I guess this new template template parameters code is going to be blocking until clang-19 becomes broadly available. My xcode toolchain is not the latest 16.2 (it is 16.0) and I kinda have reason to stick to it (safe for language bindings such as kotlin-native and tooling). Maybe some other people too.

edit: in xcode 16.0 the clang version is 16.0.0 (clang-1600.0.26.3)

cjappl · 2024-12-22T15:35:26Z

According to my experiment at Compiler Explorer, clang earlier than 19 had this issue. clang-19 successfully compiles it. https://godbolt.org/z/o755bnvPh

I guess this new template template parameters code is going to be blocking until clang-19 becomes broadly available. My xcode toolchain is not the latest 16.2 (it is 16.0) and I kinda have reason to stick to it (safe for language bindings such as kotlin-native and tooling). Maybe some other people too.

edit: in xcode 16.0 the clang version is 16.0.0 (clang-1600.0.26.3)

Yeah, we should for sure do something that works in compilers older than 19. Thanks for looking.

Let me see if there is a different way to do the same thing!

cjappl · 2024-12-24T19:40:57Z

Closing this in favor of #33 and #19

atsushieno force-pushed the concurrency-options branch 2 times, most recently from f0511b0 to 06f3f56 Compare December 17, 2024 13:57

Add Concurrency option and MPSC support using farbot.

13f13fe

context: cjappl#6 cjappl#14

atsushieno force-pushed the concurrency-options branch from 06f3f56 to 13f13fe Compare December 17, 2024 13:59

InternalQueue was missing virtual destructor.

e6ee502

cjappl reviewed Dec 20, 2024

View reviewed changes

atsushieno added 2 commits December 21, 2024 17:57

Add static assertion on MaxNumMessages when farbot MPSC queue is used.

9173032

Specify explicit failure modes on farbot fifo.

46e6306

atsushieno force-pushed the concurrency-options branch 3 times, most recently from 4ab0c94 to 1d2d9bb Compare December 22, 2024 11:46

Merge branch 'main' into concurrency-options

d1839e5

atsushieno force-pushed the concurrency-options branch from 1d2d9bb to d1839e5 Compare December 22, 2024 11:54

cjappl closed this Dec 24, 2024

Conversation

atsushieno commented Dec 17, 2024

Uh oh!

cjappl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjappl commented Dec 21, 2024

Uh oh!

atsushieno commented Dec 22, 2024

Uh oh!

atsushieno commented Dec 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjappl commented Dec 22, 2024

Uh oh!

cjappl commented Dec 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

atsushieno commented Dec 22, 2024 •

edited

Loading